Pull Request Overview

This PR adds a new Scripture Forge Project Data Provider (SF-PDP) as a standalone Node.js process that can be forked from the main scripture-forge extension to handle project data processing tasks in isolation.

• Implements a child process architecture for handling Scripture Forge project data operations
• Adds WebSocket-based communication with Scripture Forge backend for real-time collaboration
• Creates a comprehensive IPC messaging system for parent-child process communication

Reviewed Changes

Copilot reviewed 48 out of 51 changed files in this pull request and generated 5 comments.

}

      ? crypto.createHash('md5').update(options.body.toString()).digest('hex').substring(0, 8)

Reviewed 49 of 51 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 29 unresolved discussions (waiting on @lyonsil)

src/scripture-forge/src/projects/child-process-handler.ts line 14 at r2 (raw file):

import ScriptureForgeApi from './scripture-forge-api.model';

// PAPI websocket port - It would be good to get this from some config instead of hardcoding it

NIT add "TODO" to this comment so it can be found that way. Either that or just pull it from an environment variable (process.env.PAPI_WEBSOCKET_PORT) and default it to this value.

src/messages/package.json line 11 at r2 (raw file):

  "scripts": {
    "build": "npm run build:types && npm run build:functions",
    "build:functions": "npx esbuild index.ts --bundle --minify --platform=node --outfile=dist/index.js",

BTW you don't need to prefix with npx as that is assumed in scripts. Note below you call cross-env and tsc without the npx prefix.

package.json line 8 at r2 (raw file):

  "license": "MIT",
  "scripts": {
    "build": "npm run build:messages && npm run bundle:sf-pdp && webpack",

BTW I like the reordered scripts as it is clearer, however, reordering these may cause git conflict issues when updating from the template.

package.json line 19 at r2 (raw file):

    "bundle:sf-pdp": "npm run build:sf-pdp && npm run copy:sf-pdp",
    "bundle:sf-pdp:production": "npm run build:sf-pdp:production && npm run copy:sf-pdp",
    "copy:sf-pdp": "mkdir -p src/scripture-forge/assets/sf-pdp && cp -r src/sf-pdp/dist/* src/scripture-forge/assets/sf-pdp/ && mkdir -p src/scripture-forge/assets/sf-pdp/node_modules/sf-pdp-messages && cp -r src/messages/dist/* src/scripture-forge/assets/sf-pdp/node_modules/sf-pdp-messages/ && cp src/messages/package.json src/scripture-forge/assets/sf-pdp/node_modules/sf-pdp-messages/",

Pretty sure cp won't work in Windows unless they have PowerShell as the default shell (maybe it is these days?). A cross-OS generic solution is to install https://www.npmjs.com/package/shx and replace cp with shx cp. A more targeted solution is https://www.npmjs.com/package/copyfiles.

package.json line 112 at r2 (raw file):

    "zip-folder-promise": "^1.2.0"
  },
  "workspaces": ["src/*", "sf-pdp", "messages"],

BTW can you explain why this addition is necessary? As far as I can see they are both included in the original "src/*".

Code quote:

, "sf-pdp", "messages"

tsconfig.lint.json line 3 at r2 (raw file):

{
  "extends": "./tsconfig",
  "include": [".eslintrc.cjs", "*.cjs", "*.ts", "*.js", "lib", "messages", "src", "webpack"]

I think you can revert this since "messages" is a subfolder of "src".

src/messages/tsconfig.json line 21 at r2 (raw file):

  "include": ["./**/*.ts"],
  "exclude": ["node_modules", "dist"]
}

FYI This and other tsconfig files should probably utilize Project References to improve build performance.

src/sf-pdp/.eslintrc.json-old line 0 at r2 (raw file):
Can this file be deleted?

src/scripture-forge/src/auth/server-configuration.model.ts line 41 at r2 (raw file):

    scriptureForge: {
      domain: 'https://scriptureforge.org',
      webSocket: 'wss://scriptureforge.org/ws',

BTW this seems odd that it's not the same ending path as the QA one above?

src/messages/pong-message.ts line 5 at r2 (raw file):

export type PongMessage = SfPdpIpcMessage & {
  type: 'pong';
  timestamp: number;

This can be removed since it is already included in SfPdpIpcMessage.

Code quote:

timestamp: number;

src/sf-pdp/src/papi-websocket/rpc-client.ts line 36 at r2 (raw file):

  connectionStatus: ConnectionStatus = ConnectionStatus.Disconnected;
  private ws: WebSocket | undefined;
  private webSocketPort: number;

NIT this could be declared readonly.

src/sf-pdp/src/papi-websocket/rpc-client.ts line 228 at r2 (raw file):

}

export default RpcClient;

NIT do we really need the default export since we already have a named export?

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 231 at r2 (raw file):

   */
  setRateLimitInterval(intervalMs: number): void {
    this.#rateLimitIntervalMs = intervalMs;

BTW why do we need true runtime privacy on #rateLimitIntervalMs if we have this method to set it?

Code quote:

#rateLimitIntervalMs

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 388 at r2 (raw file):

   * Runs [`fetch`](https://developer.mozilla.org/en-US/docs/Web/API/Window/fetch) with Scripture
   * Forge authorization. Attempts to refresh the access token if needed. Logs out automatically if
   * unauthorized. Includes rate limiting to prevent too many requests to the same URL within a

Minor can you tell me why rate limiting was added?

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 401 at r2 (raw file):

    // Create a cache key that includes URL and relevant request options that might affect the response
    const cacheKey = ScriptureForgeAuthenticationProvider.#createRequestCacheKey(fullUrl, options);

BTW every time you call fetchWithAuthorization won't you get a different cacheKey? At least it will if options.body exists right? And if it does caching won't work here.

src/sf-pdp/src/sf-backend/share-db-websocket-adapter.ts line 50 at r2 (raw file):

export class ShareDBWebsocketAdapter extends WebSocket {
  /** The listeners to call before sending an op. */
  private beforeSendOpListeners: ((collection: string, docId: string) => Promise<void>)[] = [];

NIT can be declared readonly.

src/sf-pdp/src/papi-websocket/util.ts line 30 at r2 (raw file):

 * @returns Full requestType for use in network calls
 */
export function serializeRequestType(category: string, directive: string): SerializedRequestType {

Pretty sure we have serializeRequestType and deserializeRequestType code elsewhere (and with tests). Is it available such that it can be used here?

src/sf-pdp/src/papi-websocket/util.ts line 59 at r2 (raw file):

 * "this" will refer to the entity running the callback.
 */
export function bindClassMethods<T extends object>(this: T): void {

FYI I believe you can achieve this explicitly within a class by using arrow function declarations instead of normal function declarations. E.g. instead of this:

class MyClass {
  unboundMethod() {
    ...
  }
}

do this:

class MyClass {
  boundMethod = () => {
    ...
  };
}

src/sf-pdp/src/index.ts line 15 at r2 (raw file):

import { AsyncVariable, getErrorMessage } from 'platform-bible-utils';
import { RpcClient } from './papi-websocket/rpc-client';
import * as SfBackend from './sf-backend/scripture-forge-back-end-connection';

NIT our style convention for const is camelCase or UPPER_SNAKE_CASE. Whatever you use it shouldn't be PascalCase since it isn't a type or React component. Personally I use UPPER_SNAKE_CASE if it is truly constant so not for objects unless they are frozen. If objects aren't frozen then I use camelCase since the contents can change. Here it isn't strictly a const so there is wiggle room. Also TBH I'd just import the only exported item directly to reduce the number of named things.

src/sf-pdp/src/index.ts line 36 at r2 (raw file):

class SfPdpProcess {
  private nextMsgId: number = 0;
  private config: SfPdpConfig = {};

NIT this could be declared readonly.

src/sf-pdp/src/index.ts line 52 at r2 (raw file):

    log.info('Starting SF-PDP process', { pid: process.pid });

    if (!process.send) this.exitProcess('process is not forked, cannot continue', 100);

NIT perhaps use const for these magic numbers. Here and below.

Code quote:

100

src/sf-pdp/src/index.ts line 205 at r2 (raw file):

}

export { SfPdpProcess };

Typically it is cleaner/clearer to put the export on the class declaration.

src/sf-pdp/src/index.ts line 206 at r2 (raw file):

export { SfPdpProcess };
export type { SfPdpConfig };

Typically it is cleaner/clearer to put the export on the type declaration.

src/sf-pdp/README.md line 24 at r2 (raw file):

# Watch for changes during development
npm run watch

Remove this since there isn't a watch script.

src/sf-pdp/README.md line 34 at r2 (raw file):

# Run in development mode with ts-node
npm run dev

Remove this since there isn't a dev script.

src/sf-pdp/README.md line 46 at r2 (raw file):

```typescript
interface SfPdpMessage {

This and below looks out-of-date with the current implementation in src/messages. Please update it.

src/sf-pdp/src/sf-backend/utils.ts line 66 at r2 (raw file):

 *   fails.
 */
export function tryParseJSON<T>(value: unknown): T | undefined {

NIT should be named tryParseJson according to our style guide. However if this is coped from elsewhere perhaps leave it.

Code quote:

tryParseJSON

src/sf-pdp/src/sf-backend/scripture-forge-back-end-connection.ts line 194 at r2 (raw file):

 * data over the WebSocket.
 */
export const ScriptureForgeBackEndConnection = {

NIT our style convention for const is camelCase or UPPER_SNAKE_CASE. Whatever you use it shouldn't be PascalCase since it isn't a type or React component. Personally I use UPPER_SNAKE_CASE if it is truly constant so not for objects unless they are frozen. If objects aren't frozen then I use camelCase since the contents can change.

src/sf-pdp/src/papi-websocket/rpc.model.ts line 22 at r2 (raw file):

 * communicate on the network
 */
export enum ConnectionStatus {

Enums are a pain in JS so unless the JSON RPC interface requires an enum I would use a string union instead, i.e.:

export type ConnectionStatus = 'disconnected' | 'connecting' | 'connected';

If you must use enums it is easier to understand the status on the wire if you use string-based enums instead of number-based ones.

src/sf-pdp/src/papi-websocket/rpc.model.ts line 178 at r2 (raw file):

  // conditions. This approach is hacky but works well enough for now.
  for (let attemptsRemaining = MAX_REQUEST_ATTEMPTS; attemptsRemaining > 0; attemptsRemaining--) {
    // Intentionally awaiting inside for loop so we attempt once at a time

TYPO change "once" to "one"

Code quote:

once 

src/sf-pdp/src/pdp/pdp-factory.ts line 53 at r2 (raw file):

function generateRandomLetters(length: number): string {
  const characters = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ';

NIT it would be more performant to declare this outside the function in the module scope although probably irrelevant here since it's not likely called very much.

Reviewable status: 42 of 51 files reviewed, 22 unresolved discussions (waiting on @irahopkinson and @tjcouch-sil)

package.json line 112 at r2 (raw file):

Sorry, that was a remnant of previous work on the branch. They weren't originally in that location and then I moved them under src.

src/messages/pong-message.ts line 5 at r2 (raw file):

Done.

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 231 at r2 (raw file):

I was just trying to keep things within this class consistent. Mixing some private and some # seems unnecessarily complicating for future code reviewers/maintainers. Honestly the fact that there are two kinds of "private" is really annoying.

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 388 at r2 (raw file):

I was seeing some cases where things were asking for the same data from the same URL many times in a single second. It could also have been multiple WebViews trying to enumerate all available projects at the same time. Regardless of the reason, it seemed important for us to try to protect the servers a bit by rate limiting the REST API calls to the exact same endpoints.

src/scripture-forge/src/auth/scripture-forge-authentication-provider.model.ts line 401 at r2 (raw file):

If we're sending a different body to the same URL, we probably shouldn't assume the response will be identical. The URL might just represent an API, and if the parameters are in the body, we would want to have different cache keys for those.

src/scripture-forge/src/auth/server-configuration.model.ts line 41 at r2 (raw file):

You can talk to your former teammates about this. 😄 What I have here is accurate I'm fairly certain.

src/scripture-forge/src/projects/child-process-handler.ts line 14 at r2 (raw file):

Good idea on the TODO. Currently for the other processes, this comes from rpc.model.ts (TS/JS) or is hard-coded like it is here (C#). If we're going to fix this, I think we'll want to coordinate it in a way that works for all our use processes consistently.

src/sf-pdp/README.md line 24 at r2 (raw file):

Done.

src/sf-pdp/README.md line 34 at r2 (raw file):

Done.

src/sf-pdp/README.md line 46 at r2 (raw file):

Done.

tsconfig.lint.json line 3 at r2 (raw file):

Agreed

src/sf-pdp/.eslintrc.json-old line at r2 (raw file):

Done.

Looking better.

@irahopkinson reviewed 7 of 9 files at r3, 6 of 6 files at r4, all commit messages.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @lyonsil and @tjcouch-sil)

src/sf-pdp/README.md line 46 at r2 (raw file):

Thanks, what about the type property in this interface?

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @irahopkinson and @tjcouch-sil)

src/sf-pdp/README.md line 46 at r2 (raw file):

Fixed

src/sf-pdp/src/index.ts line 15 at r2 (raw file):

Yeah, I don't remember why I did it this way. I'm sure it was just some intermediate step, and it doesn't make sense anymore. I just changed to use the one named export from that file.

src/sf-pdp/src/index.ts line 36 at r2 (raw file):

Done

src/sf-pdp/src/index.ts line 52 at r2 (raw file):

I was just trying to pick something different for each one, not that each number would have any particular meaning. If we see the process go down for some reason, it's helpful to know what the exit code means.

src/sf-pdp/src/index.ts line 205 at r2 (raw file):

Done

src/sf-pdp/src/index.ts line 206 at r2 (raw file):

Done

src/sf-pdp/src/papi-websocket/rpc-client.ts line 36 at r2 (raw file):

This file was largely copied from paranext-core. I am trying to not change it much.

If we ever fully publish support libs from core, I would replace this class with the same one from the lib.

src/sf-pdp/src/papi-websocket/rpc-client.ts line 228 at r2 (raw file):

This file was largely copied from paranext-core. I am trying to not change it much.

If we ever fully publish support libs from core, I would replace this class with the same one from the lib.

src/sf-pdp/src/papi-websocket/rpc.model.ts line 22 at r2 (raw file):

This file was largely copied from paranext-core. I am trying to not change it much.

If we ever fully publish support libs from core, I would replace this class with the same one from the lib.

src/sf-pdp/src/papi-websocket/rpc.model.ts line 178 at r2 (raw file):

This file was largely copied from paranext-core. I am trying to not change it much.

If we ever fully publish support libs from core, I would replace this class with the same one from the lib.

src/sf-pdp/src/papi-websocket/util.ts line 30 at r2 (raw file):

This utils file was created by copying functions from other places in core. I am trying to not change it much.

If we ever fully publish support libs from core, I would replace these functions with the same ones from the lib.

src/sf-pdp/src/papi-websocket/util.ts line 59 at r2 (raw file):

This utils file was created by copying functions from other places in core. I am trying to not change it much.

If we ever fully publish support libs from core, I would replace these functions with the same ones from the lib.

src/sf-pdp/src/pdp/pdp-factory.ts line 53 at r2 (raw file):

Done

src/sf-pdp/src/sf-backend/scripture-forge-back-end-connection.ts line 194 at r2 (raw file):

Changed to UPPER_SNAKE_CASE and froze the object. This actually was originally a class, and later I realized it shouldn't be because we wanted to only have a single connection going at a time to the SF back end. I changed it to a module and didn't change the casing.

src/sf-pdp/src/sf-backend/share-db-websocket-adapter.ts line 50 at r2 (raw file):

Done

src/sf-pdp/src/sf-backend/utils.ts line 66 at r2 (raw file):

It was copied from sample code provided by the SF team. I went ahead and renamed it here since we are only using it other sample code they provided.

Reviewable status: 44 of 51 files reviewed, 5 unresolved discussions (waiting on @irahopkinson and @tjcouch-sil)

package.json line 19 at r2 (raw file):

Good call - fixed by moving this all into a TS script.

src/messages/tsconfig.json line 21 at r2 (raw file):

I would like to leave this as-is for now. It builds properly and isn't that slow. Changing tsconfig can become tricky very fast, and I don't think we're up against anything that needs this kind of change.

thanks for all this work - will be nice to have this in.

@irahopkinson reviewed 11 of 11 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @tjcouch-sil)